Skip to content

Feature/ball ready green color #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jeshernandez wants to merge 4 commits into from
Closed

Feature/ball ready green color #166

jeshernandez wants to merge 4 commits into from

Conversation

@jeshernandez
Copy link
Collaborator

@jeshernandez jeshernandez commented Dec 3, 2025

Description

What does this PR do?

Why is this change needed?

Related Issue(s)

Closes #

Changes Made

  • Basic refactor of dashboard.js ball message if statement into CONST (BALL_STATUS_RULES)
  • Ball is ready shows Green circle
  • Image ball is ready is now displayed
  • Image ball is hit is now displayed
image image image

Testing Performed

Test Environment

  • Pi Model:
  • Camera Type:
  • OS Version:
  • Installation Method:

Test Results

  • pitrac test hardware passes
  • pitrac test camera passes (if camera-related)
  • pitrac test pulse passes (if strobe-related)
  • Simulator integration tested with:
  • Performance metrics:

Test Commands Run

# Paste the actual commands and output

Performance Impact

  • Before:
  • After:
  • Impact:

Breaking Changes

  • This PR includes breaking changes
    • What breaks:
    • Migration steps:

Dependencies

  • No new dependencies

  • [ ] New dependencies added:

  • [ ] Updated dependencies:

Hardware Compatibility

  • Tested on Pi 5
  • Tested on Pi 4
  • Tested with single Pi setup
  • Tested with dual Pi setup

Documentation

  • No documentation needed
  • Documentation updated in this PR
  • Documentation PR to follow
  • Updated relevant sections:
    • README
    • Hardware guide
    • Software guide
    • Troubleshooting guide

Screenshots/Videos

Checklist

Code Quality

  • Code follows existing patterns and conventions
  • No unnecessary comments added
  • Error handling implemented appropriately

Build & Test

  • Successfully builds with ./packaging/build.sh build
  • All existing tests pass
  • New tests added for new functionality
  • Tested on actual Raspberry Pi hardware (not just CI)

Submission Requirements

  • Commits squashed if needed (git rebase -i HEAD~n)
  • CLA signed
  • PR title follows format: [PR TYPE] Brief description
  • Branch is up-to-date with main

Additional Context

@jeshernandez jeshernandez force-pushed the feature/ball-ready-green-color branch from a068f8b to e6e7f99 Compare December 3, 2025 04:43
connorgallopo
connorgallopo requested changes Dec 3, 2025
View reviewed changes
Copy link
Collaborator

@connorgallopo connorgallopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Jesse! This is looking good, a lot cleaner and more maintainable than the if/else chain.

Could you remove the CODEOWNERS changes from this PR? Codeownership generally reflects sustained contributions and deep familiarity with an area - understanding the architecture, reviewing PRs, being around when things break. It tends to develop naturally over time rather than being added alongside feature work.

The dashboard changes look good to merge once that's split out + some clarity on the other comments.

@connorgallopo
Copy link
Collaborator

connorgallopo commented Dec 3, 2025

Also - not sure if i'm missing something but wasn't the ball already green? I don't see any changes that would change the ready color of the ball status indicator

@jeshernandez
Copy link
Collaborator Author

jeshernandez commented Dec 3, 2025 •
edited
Loading

" Codeownership generally reflects sustained contributions and deep familiarity with an area - understanding the architecture, reviewing PRs, being around when things break. It tends to develop naturally over time rather than being added alongside feature work" Interesting take. I can't say I wasn't here contributing before you, and very familiar with what was added before you. But sure. I removed myself.

One major issue I see here is that I do not see peer reviews when you or james merge code. Sometimes not even feature branches. Part of a "team" is to have such reviews. Example, I tested james patch earlier today. I also tested your package issue, which I still am not able to successfully resolved. You mention James implemented and worked, and therefore you pushed your PR into main without review. I asked James, and he mentioned he had not done a clean install in a couple weeks.

  • The ball was ready stated was being muted to use the class that changed the color indicator. Now it works, before it was gray.
  • I was testing some other approaches to display the ball after hit. Dead code removed.

connorgallopo
connorgallopo approved these changes Dec 3, 2025
View reviewed changes
Copy link
Collaborator

@connorgallopo connorgallopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for addressing those edits

@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@connorgallopo connorgallopo connorgallopo approved these changes

@jamespilgrim jamespilgrim Awaiting requested review from jamespilgrim jamespilgrim is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jeshernandez @connorgallopo